test: add url type check in Module options #18664
Conversation
BridgeAR
added
author ready
|
Hey.. thanks for the approval. Are CI tests triggered automatically after every PR is made? Thanks! |
The code coverage in `root/internal/vm/Module.js` lacked test coverage for the url options paramter. The test adds a check to ensure error is thrown.
|
@jiaherrt right now you do not have to do anything further. This is going to stay open a bit longer and if no one else disagrees a collaborator is going to land this commit at some point. |
|
Alright. thanks! |
|
My pleasure. |
|
Is there any recommended direction to get more involved? |
|
We have a few issues marked as good first contribution. Besides that I recommend to just look around in code that you are interested in. Writing some tests in the beginning and improving the coverage with that is probably a good start to get used to the code base and to get to know more about how everything works together. @Trott might have some other tasks as well. Right now I am too tired to be able to think of any other further tips though. @devsnek ? |
|
i'm probably not a good person to ask, my first pr was so controversial it got elevated to a tc39 meeting topic 😆. i generally just find either
then i just make as much of it as i can and open a pr, then node collabs will help you get the pr to where it needs to be to be merged. if you're new to programming with node.js or programming in general sticking to the link that bridgear can be really good for easing you into both the node ecosystem and our codebase before you start making larger changes. generally if you're new someone in the collab team who is interested in what you're changing will kinda mentor you. even if your pr gets rejected your changes can inspire other prs that might really improve node. |
|
Alright. thanks again. |
|
ci lite for the amended commit: https://ci.nodejs.org/job/node-test-pull-request-lite/166/ |
|
landed in edffad0 |
The code coverage in `root/internal/vm/Module.js` lacked test coverage for the url options paramter. The test adds a check to ensure error is thrown. PR-URL: #18664 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
|
This will need to be manually backported to v9.x. |
The code coverage in `root/internal/vm/Module.js` lacked test coverage for the url options paramter. The test adds a check to ensure error is thrown. PR-URL: nodejs#18664 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com>
The code coverage in `root/internal/vm/Module.js` lacked test coverage for the url options paramter. The test adds a check to ensure error is thrown. PR-URL: nodejs#18664 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Hi. this is my first time contributing to an open source project. I'm a newbie here.
From the Node.js Nightly Code Coverage, I noticed that
the code coverage in
root/internal/vm/Module.jslacked some test coveragefor the url options parameter. The test just adds a check to ensure an error is thrown.
The change is a relatively small one. I took a look at the other lack of test areas for that
file, but perhaps I'm not familiar with the codebase to know how to test those.
Any help would be appreciated.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)